fix(id): drop stale telemetry and RC tag on identity refresh - #9814
Open
litianningdatadog wants to merge 44 commits into
Conversation
…VM clone resume
Firecracker snapshots a process's full memory — including OpenSSL's DRBG
state, the id.js batch buffer and its cursor, and the uuid() call's
internal buffer. Every clone that resumes from the same snapshot starts
from the identical PRNG state, so all clones produce the same trace/span
IDs, runtime-id, and RC client ID until those states happen to diverge.
Root cause: three independent entropy consumers are all frozen in the snapshot:
1. id.js pseudoRandom() — batch-fills 8192 IDs from randomFillSync (OpenSSL DRBG)
2. runtimeId in config/index.js — uuid() at module load (OpenSSL DRBG)
3. clientId in remote_config/index.js — uuid() at module load (OpenSSL DRBG)
The kernel CSPRNG is the only source that is re-seeded per clone: the
hypervisor bumps VMGenID on snapshot restore, which the Linux kernel uses
to refresh /dev/urandom before the cloned process resumes. This means a
single read from /dev/urandom after restore yields entropy unique to that
clone, regardless of what OpenSSL's DRBG is doing.
The fix is a one-time reseed of all three consumers when the VM starts —
specifically, when the Lambda MicroVM /run lifecycle hook fires
(signalled via the http.server.request.start diagnostics channel for
HTTP-server apps, or SIGUSR2 from serverless-init for others).
Changes:
id.js
- Add a swappable fill variable (default: randomFillSync). reseed()
opens /dev/urandom once, permanently swaps fill to fillFromKernel,
and resets the batch cursor to 0 so the next pseudoRandom() call
draws a full fresh batch (8192 IDs) from kernel entropy.
- Add kernelUUID() — generates a RFC 4122 v4 UUID by reading 16 bytes
directly from /dev/urandom. Used by the two refresh functions so
that runtimeId and clientId are also drawn from kernel entropy
rather than the frozen OpenSSL DRBG. Falls back to randomFillSync
on non-Linux or when /dev/urandom is unavailable.
- fillFromKernel() is defensive: closes and disables the fd on any
read failure so the hot path never retries a broken fd.
config/index.js
- Change const RUNTIME_ID to let runtimeId so it can be reassigned.
- Add refreshRuntimeId(config) — calls kernelUUID() and writes the
new value into config.tags['runtime-id'], which propagates
immediately to all subsequent spans and telemetry.
remote_config/index.js
- Change const clientId to let so it can be reassigned.
- Add refreshClientId(config) — calls kernelUUID() and updates the
module-level clientId (read by the RC client on every poll via
the get id() getter) and config.tags['_dd.rc.client_id'].
proxy.js
- When AWS_LAMBDA_MICROVM_IMAGE_ARN is set, init() registers
_registerMicroVmRunHook() which subscribes to the
http.server.request.start channel (POST /run) and process SIGUSR2.
- #refreshIdentity() calls reseed() first (opens /dev/urandom,
switches the fill source) then refreshRuntimeId and
refreshClientId, which in turn call kernelUUID() — so all three
consumers draw from the same kernel entropy that is unique per clone.
- Call order matters: reseed() must run before the uuid generators so
kernelUUID()'s fillFromKernel has a valid fd.
- A shared done flag prevents double-fire when both the HTTP channel
and SIGUSR2 arrive for the same /run event. The SIGUSR2 listener
is kept registered for the VM's lifetime because removing the only
handler reverts to the OS default action (process termination).
- Add resetRuntimeId() as a public escape hatch for apps without an
HTTP server.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Before this change, state.client.id and client_tracer.runtime_id were
plain value properties set at RemoteConfig construction time. This meant
that after refreshClientId() and refreshRuntimeId() updated the module-
level clientId and config.tags['runtime-id'], the running RC instance
kept sending the pre-snapshot values in every poll payload (getPayload()
calls JSON.stringify(this.state) on each poll).
Convert both to live getters — the same pattern already used by
config_states — so that every JSON.stringify call during a poll reads the
current value:
get id () { return clientId }
get runtime_id () { return config.tags['runtime-id'] }
No performance concern: RC polls every 5 s in the background and
JSON.stringify dominates the cost of getPayload(). The config_states
getter on the same object already breaks the hidden-class fast path.
Also corrects the JSDoc on refreshClientId, which previously claimed the
getter existed when it did not.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The fd cleanup branch inside fillFromKernel (readSync returns 0, or
readSync throws after the fd was already opened) was not exercised by
existing tests — the prior test only covered the openSync-throws case
where the fd never opens at all.
Two new cases in the reseed() describe:
- readSync returns 0 bytes: simulates a broken fd that opens but yields
nothing. Verifies closeSync is called, urandomFd is disabled, and
randomFillSync is used as fallback.
- readSync throws: simulates an EIO mid-read. Same recovery assertions.
Both cases share the same invariant: fillFromKernel permanently disables
the broken fd (urandomFd = -1) and falls back to randomFillSync, so the
application never crashes over ID generation regardless of kernel fd state.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…RunHook - Rename _registerMicroVmRunHook to #registerMicroVmRunHook (private method) - Remove redundant Boolean() wrappers in process.env guards - Trim chatty AI-generated JSDoc across id.js, proxy.js, config/index.js, and remote_config/index.js to concise, human-readable descriptions Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- id.js: uppercase hex literals (unicorn/number-literal-case) - proxy.js: use dc-polyfill instead of diagnostics_channel (n/no-restricted-require) - proxy.spec.js: update mock key from diagnostics_channel to dc-polyfill - remote_config/index.spec.js: remove redundant no-new constructor calls Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add DatadogTracer#refreshMetadata and call it from proxy.js#refreshIdentity so that the libdatadog process-discovery record is updated with the new runtime-id after a snapshot restore. Replaces the _inmem_cfg handle so the old memfd is released and only the updated record remains alive. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
… /dev/urandom directly AWS's own MicroVM guidance lists crypto.randomBytes/crypto.randomUUID as CSPRNGs safe across snapshot resume, and the Lambda base image's OpenSSL auto-reseeds on resume, making the hand-rolled /dev/urandom fd read in id.js unnecessary complexity. id.js: drop fillFromKernel/kernelUUID/urandomFd and the fs import. reseed() now just resets the batch cursor so the next pseudoRandom() call re-invokes randomFillSync(). config/index.js, remote_config/index.js: refreshRuntimeId/refreshClientId call the existing uuid() (vendored crypto-randomuuid) instead of the removed id.kernelUUID(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
proxy.js#refreshIdentity directly imported and called id.reseed(), config.refreshRuntimeId(), and remote_config.refreshClientId(). Replace those direct calls with a publish to a new dc-polyfill channel, datadog:identity:update, so proxy.js no longer needs to know those three modules exist. microvm-identity-refresh.js subscribes to the channel and calls the three producers in order. tracer.refreshMetadata(config) stays a direct call in #refreshIdentity for now — converting it to a subscriber of a downstream datadog:identity:refresh event is left to the follow-up PR (#9355) that already implements that conversion alongside three more subsystems; duplicating it here would create avoidable rebase friction between the two PRs. Also fixes a latent crash: #refreshIdentity called this._tracer?.refreshMetadata(config), but this._tracer is never null/undefined (NoopProxy's constructor always sets it to a NoopTracer instance; #updateTracing only replaces it with a real DatadogTracer when DD_TRACE_ENABLED !== false). So the optional-chaining guard never actually short-circuited, and NoopTracer had no refreshMetadata method to call — any MicroVM customer running with DD_TRACE_ENABLED: false would crash on /run. Add a no-op refreshMetadata() to NoopTracer, matching its existing pattern for every other DatadogTracer method, and drop the now-unnecessary optional chaining. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…modules Each of id.js, config/index.js, and remote_config/index.js now subscribes its own refresh function directly to the datadog:identity:update channel, instead of routing through a centralized microvm-identity-refresh.js. Also lazily generates the process-wide runtime ID on first access instead of at module load. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- tracer.js: refreshMetadata() now checks _inmem_cfg === undefined instead of a falsy check, matching the constructor's semantics so a valid-but-falsy storeMetadata() handle isn't mistaken for unset. - remote_config/index.js: client_tracer.tags is now a live getter like runtime_id and id, so refreshRuntimeId()/refreshClientId() keep the RC payload's tags array consistent with the rest of the payload. - index.d.ts / index.d.v5.ts: add the public resetRuntimeId() method that was missing from the TypeScript surface. - remote_config/index.spec.js: rewrite the clientId live-getter test to actually trigger a refresh and assert the same instance reflects it, instead of comparing two freshly-constructed instances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
We don't want to let users manually trigger a runtime-id/RC-client-id reset yet. The automatic MicroVM /run HTTP hook (#registerMicroVmRunHook) is untouched and remains the only trigger path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each of these is only ever invoked internally via its module's own subscription to the datadog:identity:update channel; the module-level export existed solely so tests could call it directly. Tests now trigger the same behavior through the channel, matching the real production entry point (proxy.js publishes to it on MicroVM /run). Two remote_config assertions that pinned an exact uuid value on the published config object are loosened to "changed from the original value", since other RemoteConfig instances left subscribed by earlier tests also react to the same publish and can win the race to set it. Addresses BridgeAR's "Do not export" review comments on PR #9075. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Avoids bypassing the eslint-process-env guardrail with an inline disable comment, matching the existing AWS_LAMBDA_FUNCTION_NAME pattern used elsewhere in the codebase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drop the direct this._tracer.refreshMetadata() call from proxy.js's MicroVM /run hook - it now only publishes to datadog:identity:update. Wiring refreshMetadata back up as a channel subscriber is deferred to #9355, which converts tracer.js and three other subsystems the same way. Remove the now-unreachable no-op refreshMetadata() from NoopTracer, since nothing calls it directly anymore. Fix a listener leak in RemoteConfig: the datadog:identity:update subscription had moved into the constructor, so every new RemoteConfig() added a permanent, unremovable listener to the shared channel. Restore the single module-level subscription (matching the id.js/config/index.js pattern), while keeping client_tracer.tags cached and refreshed only on identity update. Update proxy.spec.js to assert the publish payload directly instead of the removed refreshMetadata call, and add remote_config/index.spec.js coverage for the tags cache invalidation.
DatadogTracer#refreshMetadata() had no caller left in this PR once proxy.js's direct call moved to the diagnostic channel - #9355 is what wires it up via datadog:identity:refresh. Keeping the method (and its direct-call tests) here means it ships as dead code if #9075 lands before #9355. Moving it there keeps this PR scoped to reseeding id/runtime-id/clientId, and makes #9355 self-contained for the metadata-refresh feature it actually uses.
crypto.randomUUID() batches entropy for 128 UUIDs at a time and only refills the buffer once exhausted. If a MicroVM snapshot is taken mid-batch, every clone resuming from it reads the same cached bytes at the same cursor position, producing identical runtime-id/RC client_id values across clones despite the reseed. Pass disableEntropyCache: true so each refresh call draws fresh bytes from the kernel CSPRNG, which Firecracker/Lambda's base image reseeds on resume. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…l subscribers
refreshRuntimeId and refreshIdentity call uuid({ disableEntropyCache: true })
synchronously inside a datadog:identity:update subscriber. diagnostics_channel
does not catch subscriber exceptions, and the publish is triggered from
Node's own http.server.request.start channel (proxy.js), outside any
dd-trace try/catch. A thrown error would surface as an uncaught exception on
the first request after a MicroVM clone resumes, and would also stop any
subscriber registered after the throwing one from running.
Wrap each subscriber body in its own try/catch so a refresh failure is
logged instead of crashing the process, and so the other subscriber keeps
running independently.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ne resume (option B)
proxy.js#refreshIdentity already reseeds id.js, config.tags['runtime-id'], RC's clientId,
and process-discovery metadata on the Lambda MicroVM /run hook. A follow-up review found
more subsystems that copy those tags by value and never see the refreshed identity.
- telemetry/session-propagation.js, exporters/agentless/index.js, and
ci-visibility/exporters/agentless/writer.js + encode/agentless-ci-visibility.js cached a
copied primitive instead of holding a live reference to config/config.tags; switched them
to read live so they self-heal without any explicit trigger.
- The remaining subsystems bake the value into a cached structure (a hot-path tags string,
transformed OTLP resource attributes, or a worker-thread config snapshot) that can't just
read live, so they need to be told to regenerate. Instead of proxy.js calling each by name,
refreshIdentity now publishes once to a new dc-polyfill channel, 'datadog:identity:refresh',
and each subsystem subscribes to it independently at its own start-up site:
- dogstatsd.js: added DogStatsDClient#updateTags() to recompute the cached tags prefix;
the Custom Metrics client self-registers into a module-scope registry (no stop() hook
exists for it) while runtime-metrics clients subscribe/unsubscribe around their own
start()/stop().
- opentelemetry/metrics/index.js + otlp_transformer_base.js: added
updateResourceAttributes() to recompute the cached OTLP resource attributes; each
init call replaces its own prior subscription so restarts don't accumulate listeners.
- debugger/index.js: subscribes in start() and unsubscribes in cleanup(), reusing the
existing configure() hot-reload path. devtools_client/config.js's updateConfig() now
applies the incoming runtimeId (previously dropped), and devtools_client/status.js
stopped caching it in a module-level const.
This is "Option B" from the identity-refresh-gaps investigation: a shared event lets new
consumers opt themselves in instead of proxy.js reaching into each one by name. See
microvm-runtime-id-copies.md for the full investigation and the alternative "Option A"
(explicit per-subsystem calls) on the sibling branch
tianning.li/dd-trace-microvm-identity-refresh-option-a.
Also from review: MetricsAggregationClient#updateTags() now drops pending
counters/gauges/histograms too, matching the wrapped DogStatsDClient's existing drop of
buffered lines - they were recorded under the old identity and would otherwise survive to
be silently retagged at flush time instead of shipped correctly or dropped. Also tightened
the customMetricsClients comment: pruning of dead WeakRef entries only runs when this
channel fires (never outside a MicroVM), so it doesn't keep the Set itself bounded, only
the client/config a dead entry pointed to.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… encoder Addresses review feedback from BridgeAR on #9355. The CI Visibility agentless writer and encoder each pulled env/service out of the live `tags` object and passed them around as separate copies - the same stale-copy pattern this PR fixes elsewhere for runtime-id. Pass `tags` through as-is instead; `env` is now read live off `tags` at flush time. `this.service` was unused dead state, dropped instead of ported over. Also extends the exporters/agentless `metadata.env` field to the same `get env ()` live-read pattern already used for `runtimeID`, for consistency. Adds regression tests mirroring the existing runtime-id-reflects-a-later- mutation tests for both the CI Visibility encoder and the APM agentless exporter's `env` field. Also fixes ci-validation/writer.js, the other caller of AgentlessCiVisibilityEncoder that this commit's constructor-signature change missed. It still destructured runtime-id/env/service out of tags and passed those instead of { tags }, so this.tags was always undefined there and every CI-validation payload silently lost metadata['*'].env and metadata['*']['runtime-id'] - no throw, no log, and no test caught it since neither ci-validation.spec.js nor ci-validation-msgpack-to-json.spec.js assert on those fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codex flagged three subsystems where the identity-refresh work still left stale state after a MicroVM clone resume: - remote_config/index.js: state.client.client_tracer.tags was built once in the constructor from Object.entries(config.tags), so a later refreshClientId() updated the live client.id getter but left the cached tags array serializing the old _dd.rc.client_id. Converted tags to a getter, matching the existing id/runtime_id live-getter pattern already on this object. - dogstatsd.js: DogStatsDClient#updateTags() only recomputed the cached tag prefix for future calls. distribution() writes synchronously ahead of the next scheduled flush(), so a call made before an identity refresh could still ship with the old tags baked into _buffer/_queue. updateTags() now drops any buffered-but-unsent lines. - debugger/devtools_client/status.js: onlyUniqueUpdates()'s dedup key never accounted for runtime-id, so a probe status already deduped under the old identity was silently suppressed if the same probe/type/version was re-reported after a clone resume. Added a local runtimeId comparison that clears the dedup cache when it changes — self-contained in this file, no cross-thread signaling needed. Deliberately left out of scope: Codex's status.js comment also flagged jsonBuffer holding already-serialized payloads with the stale runtime-id baked in. Forcing an early flush there wouldn't actually fix the mislabeling (the JSON string is already stringified with the old id), only ship it sooner — and the window is bounded by uploadIntervalSeconds (default 1s) regardless. Not worth adding cross-thread channel plumbing to retag already-written payloads for a sub-1-second cosmetic issue. Verified config.tags['runtime-id'] cannot change outside a MicroVM environment: refreshRuntimeId() (the only writer after construction) is only called from microvm-identity-refresh.js, which is only triggered via proxy.js#refreshIdentity, both call sites of which are gated behind process.env.AWS_LAMBDA_MICROVM_IMAGE_ARN. Also from review: DogStatsDClient's _tags/_queue/_buffer/_offset are now true #private fields instead of _underscore convention - nothing in src or tests reaches into them externally, matching the repo's own preference for #private state that doesn't cross the class boundary. Pure encapsulation change, no behavior difference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…amplers Continues the identity-refresh rollout (option B) to the subsystems that still exported or resumed sampling under the pre-clone identity: - OTLP metrics: resetPendingState() was wiping ObservableCounter's delta baseline along with the sync-instrument cumulative state, so the first post-refresh export reported the full absolute reading as a delta instead of the change since the last export. Now only clears lastExportedState entries that have a matching cumulativeState entry. - OTLP logs: BatchLogRecordProcessor drops queued records on refresh instead of letting them export retagged under the new identity. - Agent/agentless trace exporters: drop the pending encoded batch on refresh via a new Writer#resetPendingBatch(). - OTLP traces, dogstatsd CustomMetrics, span stats, and the profiler recompute resource attributes/tags or drop pending state on refresh. - runtime_metrics/runtime_metrics and otlp_runtime_metrics reset event-loop/CPU/ELU sampler baselines on refresh so deltas don't span the snapshot pause. - debugger: fix a start()/stop() race where a session still waiting on detectDebuggerEndpoint() had no way to be told to stop, which also left its identity-refresh subscription dangling. Adds identity-refresh test coverage across metrics, logs, traces, dogstatsd, exporters, span stats, debugger, and profiler. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The MicroVM identity-refresh work included several mechanisms that drop or rebase pending buffered state (trace payload batches, OTel log/metric queues, DogStatsD aggregated samples, span-stats buckets, and runtime- metrics sampling baselines) on the datadog:identity:refresh channel. These solve a different problem than refreshing runtime-id/client_id: avoiding duplicate or misattributed telemetry when multiple MicroVM instances resume from the same frozen image snapshot. In every removed case, the exported output already carried the correct refreshed ID without the reset, since the relevant tag/resource-attribute is read live at flush/export time rather than baked into each buffered item early. Removing the resets narrows this PR back to ID-value correctness only; DogStatsDClient's own buffer/queue drop is kept, since DogStatsD lines bake tags into the string at write time and can't be relabeled later. See microvm-identity-refresh-followup.md (untracked, local) for the full inventory and follow-up plan. Also fixes NativeSpaceProfiler's OOM PROCESS-strategy export command, which baked runtime-id into a native monitorOutOfMemory() call once at profiler start and never refreshed it on a MicroVM clone resume (item 9 in the followup doc, previously deferred pending verification that the native binding tolerates a second registration). Confirmed safe by reading pprof-nodejs's bindings/profilers/heap.cc: MonitorOutOfMemory reuses per-isolate state, clears and rebuilds the stored export command each call, and reinstalls the near-heap-limit callback idempotently (guarded by a callbackInstalled flag), so it doesn't stack a duplicate handler. NativeSpaceProfiler#refreshTags() now re-registers the export command with fresh tags on identity refresh; Profiler's identity-refresh listener broadcasts to any sub-profiler that implements refreshTags(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Addresses three Codex review comments on the above: - space.js: added the missing JSDoc on #registerOOMExport(), documenting the replace-not-stack re-registration contract. - tracer.js: refreshMetadata() is now #refreshMetadata() - its only production caller was already the internal identity-refresh listener, so the public method existed solely for tracer.spec.js to call directly. Updated those tests to trigger via identityRefreshChannel.publish() instead, matching how the rest of this PR's identity-refresh tests work. - dogstatsd.js: a third comment reprised an earlier "converting _tags/ _queue/_buffer/_offset to #private breaks external consumers" finding. Not fixed - the specific breakage (the sirun benchmark reading _queue directly) predates the _enqueue() accessor already added for this; no other external reader exists in src, tests, or benchmarks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Also adds the JSDoc Codex flagged as missing on the new #refreshMetadata (caught immediately after the rename above landed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Addresses two more Codex review comments: - profiler.js: the identity-refresh listener called each sub-profiler's refreshTags() unguarded. diagnostics_channel.publish() does not catch subscriber exceptions, and this publish happens synchronously inside the MicroVM /run HTTP-hook handler, so a native re-registration failure in one profiler (e.g. NativeSpaceProfiler's monitorOutOfMemory()) could crash request handling instead of just leaving that profiler's tags stale. Wrapped each refreshTags() call in try/catch + log.error(), matching how Profiler#start() already contains failures from the same registration path. - debugger/index.js: a stop()+start() cycle while the first start()'s detectDebuggerEndpoint() call was still pending could let the stale callback build a worker once it finally resolved, since configChannel is non-null again by then (the new session's channel) and the existing guard couldn't tell the two sessions apart. It would mix the first session's probe/log ports with the second session's config port, and transferring an already-detached port throws DataCloneError. Added a generation counter, incremented per start() and captured by the pending callback, so a superseded callback is rejected even though configChannel looks live. Regression test uses two independently-resolvable deferred fetchAgentInfo callbacks to reproduce the exact ordering; verified it fails without the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Two clarifying comments, no logic change: - benchmark/sirun/dogstatsd/index.js: note why the fake socket only implements send/on/unref and why send() ignores everything but the buffer argument. - debugger/index.js: note why stop()'s guard also checks configChannel (catches a pending start with no worker yet, so cleanup() - and the identity-refresh unsubscribe - doesn't get skipped). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hanged updateTags() unconditionally cleared the queue/buffer/offset on every identity refresh, even when the recomputed tag prefix was identical to the cached one. In the default MicroVM config (Remote Config disabled, runtimeMetricsRuntimeId off), the tag list never actually changes, so this silently dropped buffered distributions/histograms for no reason. Only clear buffered state when the tag prefix actually changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…vate The #private conversion in 60f47c6 was a pure encapsulation nit with no behavior difference, unrelated to the identity-refresh fix itself. It also broke the dogstatsd benchmark (which reached into _buffer/_offset/ _queue directly), pulling an unrelated benchmark-script change into this PR and tripping the CI gate that blocks a PR from mixing benchmark and non-benchmark source changes. Revert to _tags/_queue/_buffer/_offset and drop the now-unneeded benchmark-script diff; the updateTags() fix logic is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Crashtracker never subscribed to the identity-refresh channel, so a crash after /run still reported the snapshot's stale runtime-id and RC client id. Subscribe once at module load, same as dogstatsd.js's pattern for a singleton with no start()/stop() to hang the subscription off of. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…encoder (#9610) config.tags is always an object by the time these run - Config#applyDefaults seeds it from DD_TAGS's default (parsed to {} for an empty string) before any other config logic, and every production caller of these constructors passes config.tags straight through. Addresses BridgeAR's "tags will always be an object" review comments on #9355 for exporters/agentless/index.js and encode/agentless-ci-visibility.js. Also drops the same redundant `?.` on DogStatsDClient's #tags, which is always populated via generateClientConfig()/buildClientConfig() (both build it as an array, never undefined). Updates one test that constructed AgentlessCiVisibilityEncoder without tags (not a shape any real caller produces) to pass tags: {} instead. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…uctor Moves the identityRefreshChannel.subscribe() call from module scope into Crashtracker's constructor, binding it to `this` instead of closing over the module-level singleton. Matches the pattern already used by DatadogTracer's constructor, and removes the risk of a differently constructed instance never receiving identity-refresh updates. Addresses: #9355 (comment)
Registers a WeakRef to the CustomMetrics instance itself instead of a
throwaway {client, config} wrapper object, and moves the tag-recompute
logic into a refreshTags() method on CustomMetrics. Removes the
#registryEntry indirection and the module-scope subscriber's reach into
the instance's private client/config.
Addresses: #9355 (comment)
…ne test Replaces nested real setTimeout waits (coupled to the 100ms production export interval) with sinon fake timers, removing the risk of CI scheduler jitter causing the wrong number of interval firings to be observed. Addresses: #9355 (comment)
A later initialization error is contained by init(), but late hook registration was skipped and left restored clones without an identity refresh.
Disabled tracing never constructs a DatadogTracer, so the MicroVM /run hook must not register process metadata for a tracer that does not exist.
Contributor
Overall package sizeSelf size: 8.03 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.3.3 | 125.43 kB | 441.68 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR follows up on MicroVM clone-resume identity refresh behavior in dd-trace-js, ensuring pre-snapshot buffered/aggregated telemetry is dropped (instead of being flushed under each clone’s refreshed identity) and fixing a remote-config client-id tag regression after identity refresh.
Changes:
- Reset/discard pending state on
datadog:identity:refreshacross span-stats buckets, agentless trace batches, OTel metrics measurement queues/cumulative baselines, and OTel logs queued records. - Rebase runtime-metrics sampler baselines (CPU/ELU/event-loop delay) on identity refresh to avoid deltas spanning the snapshot pause.
- Ensure
_dd.rc.client_idis written back intoconfig.tagswhenever an RC client exists (even if tags were rebuilt by an RC lib-config update).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dd-trace/test/span_stats.spec.js | Adds coverage for clearing pending span-stats buckets on identity refresh. |
| packages/dd-trace/test/runtime_metrics.spec.js | Adds coverage for rebasing event-loop-delay baseline on identity refresh (runtime + OTLP variants). |
| packages/dd-trace/test/remote_config/index.spec.js | Ensures RC client-id tag is restored on identity refresh when an RC client exists. |
| packages/dd-trace/test/opentelemetry/metrics.spec.js | Adds coverage for dropping pre-refresh sync Counter measurements on identity refresh. |
| packages/dd-trace/test/opentelemetry/logs.spec.js | Adds coverage for dropping queued pre-refresh log records on identity refresh. |
| packages/dd-trace/test/exporters/common/writer.spec.js | Adds unit test for discarding a pending encoded batch via resetPendingBatch(). |
| packages/dd-trace/test/exporters/agentless/exporter.spec.js | Adds coverage that agentless exporter drops pending trace batch on identity refresh. |
| packages/dd-trace/test/dogstatsd.spec.js | Adds coverage for dropping pending aggregated metrics when identity refresh changes tags, and preserving when unchanged. |
| packages/dd-trace/src/span_stats.js | Subscribes to identity refresh to drop pre-snapshot span-stats buckets. |
| packages/dd-trace/src/runtime_metrics/runtime_metrics.js | Routes identity refresh through subscribeToIdentityRefresh(..., resetSamplerBaselines) and implements baseline rebase. |
| packages/dd-trace/src/runtime_metrics/otlp_runtime_metrics.js | Rebases OTLP runtime-metrics baselines (ELU + event-loop histogram) on identity refresh. |
| packages/dd-trace/src/runtime_metrics/client.js | Extends subscribeToIdentityRefresh to accept an optional post-refresh callback. |
| packages/dd-trace/src/remote_config/index.js | Writes _dd.rc.client_id back to config.tags whenever an RC client exists. |
| packages/dd-trace/src/opentelemetry/metrics/periodic_metric_reader.js | Adds resetPendingState() to discard queued measurements and sync cumulative state. |
| packages/dd-trace/src/opentelemetry/metrics/index.js | Subscribes to identity refresh to drop pending OTel metrics state (with restart-safe unsubscribe handling). |
| packages/dd-trace/src/opentelemetry/logs/index.js | Subscribes to identity refresh to drop pending OTel logs state (with restart-safe unsubscribe handling). |
| packages/dd-trace/src/opentelemetry/logs/batch_log_processor.js | Adds resetPendingState() to discard queued log records and clear the timer. |
| packages/dd-trace/src/exporters/common/writer.js | Adds resetPendingBatch() to drop pending encoded trace batches. |
| packages/dd-trace/src/exporters/agentless/index.js | Subscribes to identity refresh to drop pending agentless trace batches. |
| packages/dd-trace/src/dogstatsd.js | Makes tag updates report whether tags changed; resets aggregation only when the underlying tag prefix changed. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
BenchmarksBenchmark execution time: 2026-08-13 19:42:00 Comparing candidate commit 579c97e in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2309 metrics, 49 unstable metrics.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 579c97e | Docs | Datadog PR Page | Give us feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## BridgeAR/2026-08-05-microvm-identity-refresh-review #9814 +/- ##
=====================================================================================
Coverage 98.56% 98.56%
=====================================================================================
Files 970 970
Lines 140495 140639 +144
Branches 12962 12382 -580
=====================================================================================
+ Hits 138482 138627 +145
+ Misses 2013 2012 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
litianningdatadog
force-pushed
the
tianning.li/2026-08-05-microvm-identity-refresh-review-followup
branch
from
August 13, 2026 19:14
4f85c4a to
016848d
Compare
litianningdatadog
changed the title
Buffered/aggregated telemetry recorded before a MicroVM snapshot would otherwise export or flush under every clone's refreshed identity instead of being dropped with the rest of the pre-clone state. Reset it as part of the identity-refresh path, in each of the affected subsystems: - dogstatsd: MetricsAggregationClient drops pending counters/gauges/ histograms when the wrapped client's tags actually change - agentless exporter: Writer#resetPendingBatch() discards the pending encoded trace batch - OTLP logs: BatchLogRecordProcessor#resetPendingState() discards queued log records and clears the batch timer - OTLP metrics: PeriodicMetricReader#resetPendingState() discards queued measurements and rebases sync Counter/Histogram cumulative state - span stats: SpanStatsProcessor replaces its bucket map - runtime metrics: rebase CPU/event-loop/ELU sampler baselines so the next collection reports a delta since the resume, not one spanning the snapshot pause Also fixes a separate identity-refresh gap: an RC lib-config update rebuilds config.tags from tracked sources (config/remote_config.js's tracing_tags transformer), dropping the directly-set _dd.rc.client_id key. refreshIdentity()'s guard only wrote the refreshed value back when the tag was already present, so once that sequence happened, config.tags (and the DogStatsD/OTLP tags built from it) permanently lost _dd.rc.client_id after an identity refresh, even though the RC client's own id field kept updating correctly. Gate the write on the RC client existing instead, and write it unconditionally in that case. SpanStatsProcessor and AgentlessExporter also subscribed to the identity-refresh channel per instance with no cleanup. Harmless in production (both are process-lifetime singletons), but each of the many instances constructed across a test run stayed subscribed forever. Now replace the previous subscription on construction, matching the pattern already used for the OTel logs/metrics initializers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
litianningdatadog
force-pushed
the
tianning.li/2026-08-05-microvm-identity-refresh-review-followup
branch
from
August 13, 2026 19:31
016848d to
579c97e
Compare
litianningdatadog
marked this pull request as ready for review
August 14, 2026 13:20
litianningdatadog
requested review from
BridgeAR
and
a balanced review from Copilot
and removed request for
a team
August 14, 2026 13:20
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/dd-trace/src/dogstatsd.js:255
- The aggregation maps also need to be discarded on every identity refresh, not only when the rendered tag prefix changes. Otherwise configurations that omit runtime-id and RC tags preserve identical pre-snapshot counters/gauges/histograms in every clone; the refresh event itself is the signal to drop them.
if (this._client.updateTags(tags)) {
this.reset()
packages/dd-trace/src/dogstatsd.js:71
- An identity refresh still represents a clone resume when the generated DogStatsD tags are unchanged—for example, with runtime-id tagging disabled and Remote Config disabled. Returning here keeps the pre-snapshot encoded buffer, so every clone can flush the same metrics. Clear the buffer on every refresh while retaining the boolean only as an indication that the prefix changed.
This issue also appears on line 254 of the same file.
if (tagsPrefix === this.#tagsPrefix) return false
packages/dd-trace/src/runtime_metrics/runtime_metrics.js:186
- On supported Node versions before the new per-iteration sampler, the default path uses
@datadog/native-metrics; itsstats()call owns and drains the CPU, event-loop, and GC accumulators. This reset only rebases JavaScript state, so the first post-resume capture still exports the native pre-snapshot accumulations from every clone. DrainnativeMetrics.stats()during refresh as well.
function resetSamplerBaselines () {
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 579c97eb74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+254
to
+256
| if (this._client.updateTags(tags)) { | ||
| this.reset() | ||
| } |
There was a problem hiding this comment.
On a MicroVM resume, the generated DogStatsD tags commonly remain unchanged: DogStatsDClient.generateClientConfig() excludes runtime-id unless runtimeMetricsRuntimeId is enabled, which defaults to false, and serverless configuration disables Remote Config so there may be no changing RC client-id tag either. In that default case updateTags() returns false and this branch retains counters, gauges, and histograms accumulated in the snapshot, causing every clone to flush duplicate pre-snapshot values; the identity-refresh subscriber should reset aggregation regardless of whether the serialized tag prefix changes. The added tests only exercise the non-default runtime-id-enabled case and explicitly preserve the faulty sibling case.
AGENTS.md reference: AGENTS.md:L127-L129
Useful? React with 👍 / 👎.
Comment on lines
+173
to
+178
| function resetSamplerBaselines () { | ||
| lastTime = performance.now() | ||
| lastElu = performance.eventLoopUtilization() | ||
|
|
||
| if (lastCpuUsage !== null) { | ||
| lastCpuUsage = process.cpuUsage() |
There was a problem hiding this comment.
When the @datadog/native-metrics branch is active, this rebases lastTime but does not consume or reset the addon's accumulated CPU, event-loop, and GC statistics. The next captureNativeMetrics() therefore reads pre-snapshot values from nativeMetrics.stats() while dividing CPU usage by only the post-refresh elapsed time, producing an inflated first sample and exporting the stale event-loop/GC data the change intends to discard. Resetting this branch needs to drain the native statistics as well; the new observer-reset test explicitly skips this supported sibling path.
AGENTS.md reference: AGENTS.md:L127-L129
Useful? React with 👍 / 👎.
Comment on lines
+224
to
+227
| for (const key of this.#cumulativeState.keys()) { | ||
| this.#lastExportedState.delete(key) | ||
| } | ||
| this.#cumulativeState.clear() |
There was a problem hiding this comment.
Clearing #cumulativeState starts a new value series for synchronous cumulative counters, histograms, and up/down counters, but MetricAggregator.#startTime remains the timestamp captured when the snapshot image initialized. After a clone resumes—potentially days later—the first cumulative point therefore contains only post-resume measurements while claiming an interval beginning before the snapshot, which yields incorrect rates and temporal metadata under the clone's new resource identity. The reset must also advance the aggregator start time; the added test covers only the default delta-counter sibling.
AGENTS.md reference: AGENTS.md:L127-L129
Useful? React with 👍 / 👎.
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Native runtime metrics still retain pre-snapshot CPU/event-loop/GC accumulators, while span stats can retain the old tags object after an RC tag update. Both paths can make clones emit telemetry associated with snapshot-era state or identity.
🤖 Datadog Autotest · Commit 579c97e · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Comment on lines
+174
to
+175
| lastTime = performance.now() | ||
| lastElu = performance.eventLoopUtilization() |
There was a problem hiding this comment.
Every clone using native runtime metrics can report the same pre-snapshot event-loop/GC activity and inflated CPU usage under its fresh identity.
Assertion details
- Input: A MicroVM snapshot taken after @datadog/native-metrics accumulates activity but before its periodic stats() collection, then resumed into one or more clones.
- Expected:
Identity refresh should drain the native addon's CPU, event-loop, and GC state before establishing post-resume baselines. - Actual:
The new refresh callback only rebases JavaScript timestamps. On the native path, the addon's CPU baseline and event-loop/GC histograms remain populated until nativeMetrics.stats() is called, so the next periodic collection includes snapshot-era activity.
Suggested change
| lastTime = performance.now() | |
| lastElu = performance.eventLoopUtilization() | |
| nativeMetrics?.stats() | |
| lastTime = performance.now() | |
| lastElu = performance.eventLoopUtilization() |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Comment on lines
+219
to
+220
| const onIdentityRefresh = () => { | ||
| this.buckets = new TimeBuckets() |
There was a problem hiding this comment.
Agent span stats from multiple clones can share the snapshot runtime ID, causing cross-clone identity collisions and incorrect aggregation.
Assertion details
- Input: Remote Config applies tracing_tags after SpanStatsProcessor construction, replacing config.tags, followed by a MicroVM identity refresh and agent-format span-stats export.
- Expected:
The span-stats processor should adopt the current config.tags object when identity refresh fires, before exporting new buckets. - Actual:
The callback clears buckets but leaves this.tags pointing to the object captured during construction. An RC tracing_tags update replaces config.tags, so identity refresh mutates a different object and subsequent v0.6 stats retain the snapshot runtime ID.
Suggested change
| const onIdentityRefresh = () => { | |
| this.buckets = new TimeBuckets() | |
| const onIdentityRefresh = (config) => { | |
| this.tags = config?.tags ?? this.tags | |
| this.buckets = new TimeBuckets() |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
BridgeAR
force-pushed
the
BridgeAR/2026-08-05-microvm-identity-refresh-review
branch
from
August 14, 2026 19:38
6cac3ff to
31556cd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Follow-up to #9709. Two fixes:
When a MicroVM clone resumes, anything buffered or aggregated before the snapshot (DogStatsD counters/gauges/histograms, agentless trace batches, OTLP log/metric queues, span-stats buckets, runtime-metrics CPU/event-loop baselines) was only getting retagged with the new identity, not dropped. Every clone would then flush the same pre-snapshot data under its own fresh runtime-id, which the backend reads as duplication rather than one event. Now each of those gets reset/rebased as part of the identity-refresh path instead of just retagged.
Separately,
_dd.rc.client_idcould go missing fromconfig.tagsafter an identity refresh: an RC lib-config update rebuildsconfig.tagsfrom tracked sources, dropping this directly-set key, and the refresh code only wrote the new client id back if the tag was already present. So after that sequence, DogStatsD/OTLP tags would silently lose_dd.rc.client_ideven though the RC client's ownidkept updating fine. Fixed by gating on the RC client existing instead of the tag's presence.Motivation
Both were flagged in Codex review on #9709 and left unaddressed — the first batch was deferred with "follow-up PR", the RC client-id one was a later comment nobody had replied to yet.
Additional Notes
Deliberately did not touch the debugger/Dynamic Instrumentation identity-refresh path. That was implemented and then removed in 632c70c as modeling unreachable state (a restored MicroVM can't have an active debugger session); not re-litigating that call here.